Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: fix nested hooks mutation #14143

Closed
wants to merge 7 commits into from
Closed

async_hooks: fix nested hooks mutation #14143

wants to merge 7 commits into from

Conversation

AndreasMadsen
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

async_hooks

In some cases restoreTmpHooks is called too early, this causes active_hooks_array to change during execution of the init hooks.

This depends on #14054

@AndreasMadsen AndreasMadsen added the blocked PRs that are blocked by other issues or PRs. label Jul 9, 2017
@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 9, 2017
@AndreasMadsen AndreasMadsen mentioned this pull request Jul 9, 2017
4 tasks
@AndreasMadsen
Copy link
Member Author

@AndreasMadsen
Copy link
Member Author


const common = require('../common');
const async_hooks = require('async_hooks');
const crypto = require('crypto');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You kept crypto here, was that intentional? If so, it probably needs a guard for skipping the test if appropriate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is from the blocking PR, I will rebase once that is fixed.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-0.1

}

function emitHookFactory(symbol, name) {
// Called from native. The asyncId stack handling is taken care of there
// before this is called.
// eslint-disable-next-line func-style
const fn = function(asyncId) {
processing_hook = true;
processing_hook += 1;
// Use a single try/catch for all hook to avoid setting up one per
// iteration.
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that doing reference counting is too fragile.
Why not replace the for either with a .forEach for with a for (const hook of Array.from(active_hooks_array))

Copy link
Member Author

@AndreasMadsen AndreasMadsen Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that doing reference counting is too fragile.

Could you be more explicit?

edit: Just to avoid confusion, this is not a reference counting it is a depth counting.

Why not replace the for either with a .forEach

.forEach is not resistant to mutation.

const a = [1,2,3];
a.forEach(function (value) {
  process._rawDebug(value);
  a.pop();
});

prints 1 2.

with a for (const hook of Array.from(active_hooks_array))

Array copy is too slow. When async_hooks is enabled this is the hottest code path we have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. I was sure .forEach iterated over a fixed array (it just ignores newly added elements)

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a 2 nits

@@ -359,9 +354,9 @@ function emitHookFactory(symbol, name) {
} catch (e) {
fatalError(e);
}
processing_hook = false;
processing_hook -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since we are going with depth counting, IMHO the decrement should be in a finally clause

Copy link
Member Author

@AndreasMadsen AndreasMadsen Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, but there is no way to escape fatalError so the code will never be executed.

The try catch finally order is:

try {
  throw new Error();
} catch (e) {
  console.log('catch');
} finally {
  console.log('finally');
}

outputs catch finally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, but there is no way to escape fatalError so the code will never be executed.

At the moment... If at some point we'll enable a fatalError trap this will break. Better to be explicit about these.
Reference/depth counting only works if it's ACID

@@ -441,7 +436,18 @@ function init(asyncId, type, triggerAsyncId, resource) {
} catch (e) {
fatalError(e);
}
processing_hook = false;
processing_hook -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally

BridgeAR and others added 6 commits July 12, 2017 18:23
This fixes an error that could occure by nesting async_hooks calls
In some cases restoreTmpHooks is called too early, this causes
active_hooks_array to change during execution of the init hooks.
for when node is compiled without crypto support
@AndreasMadsen
Copy link
Member Author

Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks much. Especially for the additional code comment in init(). Have a non-blocking nit to change the processing_hooks code comment to explain that it's a depth counter.

@@ -29,7 +29,7 @@ var active_hooks_array = [];
// Track whether a hook callback is currently being processed. Used to make
// sure active_hooks_array isn't altered in mid execution if another hook is
// added or removed.
var processing_hook = false;
var processing_hook = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly change comment above from simply

Track whether a hook [...]

to something like

Use a counter to track [...] and prevent nested calls [...]

@AndreasMadsen
Copy link
Member Author

Fixed the nit. I had to use some artistic freedom to fit in the "prevent nested calls" part. But the meaning should be the same.

CI: https://ci.nodejs.org/job/node-test-pull-request/9106/

@AndreasMadsen
Copy link
Member Author

landed in f94fd0c

@trevnorris
Copy link
Contributor

@AndreasMadsen Awesome. Thanks for adding the comment changes.

@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
In some cases restoreTmpHooks is called too early, this causes
active_hooks_array to change during execution of the init hooks.

PR-URL: #14143
Ref: #14054 (comment)
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
In some cases restoreTmpHooks is called too early, this causes
active_hooks_array to change during execution of the init hooks.

PR-URL: #14143
Ref: #14054 (comment)
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. blocked PRs that are blocked by other issues or PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants